Tagging subscribers to this area: @agocke, @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

🤖 Copilot Code Review — PR #126120

Holistic Assessment

Motivation: Justified. These CircularCctor tests create 3–4 threads to exercise circular static constructor deadlock detection. On single-threaded WASM, threading APIs throw PlatformNotSupportedException, causing crashes and deadlocks. The sibling test CircularCctorTwoThreadsThreeCC.cs was already updated with this same pattern, confirming this is a known gap.

Approach: Correct. Using [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] is the established pattern in this test directory. It is semantically superior to the previous [ActiveIssue] on Browser (used in FourThreadsBFI.il) because the issue is not a temporary bug — threading is fundamentally unavailable on single-threaded WASM. ConditionalFact cleanly skips the test rather than marking it as a known failure.

Summary: ✅ LGTM. Clean, consistent, and complete change. All multi-threading CircularCctor tests are covered (the only sibling, CircularCctorTwoThreadsThreeCC.cs, was already updated). The IL encoding follows the correct constructor signature. One minor cleanup opportunity noted below, but it is non-blocking.

Detailed Findings

✅ Correctness — ConditionalFact applied correctly

All 4 C# files correctly replace [Fact] with [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))]. The existing [ActiveIssue("needs triage", typeof(PlatformDetection), nameof(PlatformDetection.IsSimulator))] attribute is properly preserved — it covers a separate concern (simulator platforms) unrelated to threading support.

✅ IL encoding — Correct attribute encoding

The IL files correctly encode the ConditionalFactAttribute constructor:

.custom instance void [Microsoft.DotNet.XUnitExtensions]Xunit.ConditionalFactAttribute::.ctor(
    class [System.Runtime]System.Type, string[]) = {
    type([TestLibrary]TestLibrary.PlatformDetection)
    string[1] ('IsMultithreadingSupported')
}

The required assembly externs (Microsoft.DotNet.XUnitExtensions, TestLibrary, System.Runtime) are properly added. Note: this PR uses [System.Runtime]System.Type while some older IL tests (e.g., mcc_interop) use [mscorlib]System.Type — both are valid; System.Runtime is the modern canonical reference.

✅ Completeness — All multi-threaded tests covered

Verified that the CircularCctors/ directory contains exactly 9 test source files. All multi-threading tests are now covered:

• CircularCctorTwoThreadsThreeCC.cs — already had ConditionalFact (pre-existing)
• CircularCctorFourThreads.cs + FourThreadsBFI.il — updated ✓
• CircularCctorThreeThreads01/02/03.cs + 01/02/03BFI.il — updated ✓

✅ TestLibrary dependency — Correctly resolved

The .ilproj files don't explicitly reference TestLibrary, but Directory.Build.targets auto-includes $(TestLibraryProjectPath) via IsMergedTestRunnerAssembly=true (the default). This matches the existing build infrastructure.

✅ ActiveIssue removal in FourThreadsBFI.il — Appropriate

The FourThreadsBFI.il previously had:

.custom instance void [...]Xunit.ActiveIssueAttribute::.ctor(string, valuetype [...]Xunit.TestPlatforms) = {
    string('System.Threading.Thread.ThrowIfMultithreadingIsNotSupported: PlatformNotSupportedException')
    int32(0x400) // TestPlatforms.Browser
}

This was a workaround for the exact same threading issue. Replacing it with ConditionalFact(IsMultithreadingSupported) is strictly better: it covers all single-threaded platforms (not just Browser) and uses a proper skip mechanism rather than marking an expected failure.

💡 Unused xunit.core assembly extern in IL files — Minor cleanup

After removing [Fact] (which was [xunit.core]Xunit.FactAttribute), the .assembly extern xunit.core {} declaration is no longer referenced in any of the 4 IL files. This is harmless dead metadata, but could be cleaned up for tidiness. Non-blocking, fine as a follow-up.

Generated by Code Review for issue #126120 · ◷

/ba-g stucked infrastructure

ci-analysis, to be sure the runtime tests were not broken

● Let me check if any had actual test failures:

● Check all 74 work items for exit codes (shell)
│ failed_count=0
│ passed_count=0
│ for wi in $(curl -s "https://helix.dot.net/api/jobs/965928c6-fbde-4278-b6a7-4fa007837ec8/workitems?api-version=2019-06-17" | python3 -c "import json,sys; [print(w.get('Name',w.get('name',''))) for w
│ in json.load(sys.stdin)]"); do
│ result=$(curl -sL "https://helix.dot.net/api/jobs/965928c6-fbde-4278-b6a7-4fa007837ec8/workitems/$wi/console?api-version=2019-06-17" 2>/dev/null | grep "Exit Code:" | tail -1)…
└ 13 lines...

● Read shell output Waiting up to 120 seconds for command output
└ 6 lines...

● Shell "Check all 74 work items for exit codes" completed

● Yes — all real tests completed and passed. Here's the summary:

• 73/74 work items exited with code 0 (tests passed)
• 3 "non-zero" are false positives: HelixController, Work, Queueing — these are fragments of the single entry HelixController Work Queueing (a controller job, not a test)
• The AzDO job shows as CANCELLED because it hit the 3h timeout while Helix items were queued (3h26m queue delay!). Tests actually ran in ~1-3 min each.
• Exit code -4 on work items = failed to upload results back to AzDO (auth token expired after cancellation), not test failures.

Verdict: All coreclr/browser-wasm Pri0 runtime tests passed. The "failure" is purely infrastructure — AzDO timed out waiting for Helix queue, then revoked the upload token.